-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Mesh::merge
#11456
Add Mesh::merge
#11456
Conversation
_ => panic!( | ||
"Incompatible vertex attribute types {} and {}", | ||
enum_variant_name, | ||
other_values.enum_variant_name() | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to return some error instead? I'm not sure how common it would be to reach this panic since I'd expect e.g. position attributes to almost always be Float32x3
let enum_variant_name = values.enum_variant_name(); | ||
if let Some(other_values) = other.attribute(id) { | ||
match (values, other_values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, if self
and other
's attributes don't match up:
- if
self
has an attribute butother
doesn't, it is kept (note that this means future'sself.count_vertices()
will report the old vertices count); - if
other
has an attribute butself
doesn't, it is silently ignored.
Which could be kinda surprising. I don't see much better ways to handle this (other than error), in both cases it should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I just added a short mention in the doc comment that attributes of other
that don't exist on self
will be ignored
// The indices of `other` should start after the last vertex of `self`. | ||
let index_offset = self.count_vertices(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that (from count_vertices
's documentation):
If the attributes have different vertex counts, the smallest is returned.
So it may be silently wrong (for your usecase) if that's the case. Might also be something to document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to just use the vertex position attribute length for now, although I'm not entirely sure if that's fully correct either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to panic if the attributes don't match (maybe only in debug mode?). Merging meshes seems very non-arbitrary, so catching the error will be easy.
Maybe a 'force_merge' method to ignore? Seems overkill though.
IMO we should just return a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied this PR as a patch and tested in my game, works great!
I would add mention in the docs that if self
have a missing attribute or indices, they will be ignored when merging.
Since it's a very small, yet useful PR, I added it to the milestone. Feel free to remove if you think it won't fit.
This is already kinda mentioned: /// Note that attributes of `other` that don't exist on `self` will be ignored.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I somehow missed it.
Not a great milestone candidate at this stage (it's not vital), but I'm happy to merge it regardless. |
Objective
It can sometimes be useful to combine several meshes into one. This allows constructing more complex meshes out of simple primitives without needing to use a 3D modeling program or entity hierarchies.
This could also be used internally to increase code reuse by using existing mesh generation logic for e.g. circles and using that in cylinder mesh generation logic to add the top and bottom of the cylinder.
Note: This is not implementing CSGs (Constructive Solid Geometry) or any boolean operations, as that is much more complex. This is simply adding the mesh data of another mesh to a mesh.
Solution
Add a
merge
method toMesh
. It appends the vertex attributes and indices ofother
toself
, resulting in aMesh
that is the combination of the two.For example, you could do this:
This would result in
cuboid
being aMesh
that also has the cylinder mesh and torus mesh. In this case, they would just be placed on top of each other, but by utilizing #11454 we can transform the cylinder and torus to get a result like this:2024-01-21.14-31-30.mp4
This is just a single entity and a single mesh.